Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Dispatch back to IO thread for non blocking requests #20438

Merged
merged 1 commit into from
Sep 29, 2021
Merged

Dispatch back to IO thread for non blocking requests #20438

merged 1 commit into from
Sep 29, 2021

Conversation

stuartwdouglas
Copy link
Member

No description provided.

@stuartwdouglas stuartwdouglas mentioned this pull request Sep 29, 2021
Closed
@geoand
Copy link
Contributor

geoand commented Sep 29, 2021

I was thinking of the same thing actually.

My only question is, does this actually get you back to the same Event Loop thread that security was one?

@stuartwdouglas
Copy link
Member Author

It gets you back to the requests event loop thread, security is run on a worker thread. Because this uses the context from the connection it is guaranteed to be the same thread.

@geoand
Copy link
Contributor

geoand commented Sep 29, 2021

Right, I meant the Security filter that could lead to the dispatch to the worker thread.

Sounds good

@geoand
Copy link
Contributor

geoand commented Sep 29, 2021

We would likely need to do something similar in Reactive Routes, but I assume that can wait since we are deprecating them

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 29, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 649d2c7

Status Name Step Failures Logs Raw logs
✖ JVM Tests - JDK 11 Build Failures Logs Raw logs
✖ JVM Tests - JDK 17 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: integration-tests/resteasy-reactive-kotlin/standard

📦 integration-tests/resteasy-reactive-kotlin/standard

✖ io.quarkus.it.resteasy.reactive.kotlin.FlowResourceTest.testSseStrings line 22 - More details - Source on GitHub

java.util.concurrent.TimeoutException
	at java.base/java.util.concurrent.CompletableFuture.timedGet(CompletableFuture.java:1886)
	at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2021)

✖ io.quarkus.it.resteasy.reactive.kotlin.FlowResourceTest.testSuspendSseStrings line 29 - More details - Source on GitHub

java.util.concurrent.TimeoutException
	at java.base/java.util.concurrent.CompletableFuture.timedGet(CompletableFuture.java:1886)
	at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2021)

✖ io.quarkus.it.resteasy.reactive.kotlin.FlowResourceTest.testSeeJson line 36 - More details - Source on GitHub

java.util.concurrent.TimeoutException
	at java.base/java.util.concurrent.CompletableFuture.timedGet(CompletableFuture.java:1886)
	at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2021)

⚙️ JVM Tests - JDK 17 #

- Failing: integration-tests/resteasy-reactive-kotlin/standard

📦 integration-tests/resteasy-reactive-kotlin/standard

✖ io.quarkus.it.resteasy.reactive.kotlin.FlowResourceTest.testSseStrings line 22 - More details - Source on GitHub

java.util.concurrent.TimeoutException
	at java.base/java.util.concurrent.CompletableFuture.timedGet(CompletableFuture.java:1960)
	at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2095)

✖ io.quarkus.it.resteasy.reactive.kotlin.FlowResourceTest.testSuspendSseStrings line 29 - More details - Source on GitHub

java.util.concurrent.TimeoutException
	at java.base/java.util.concurrent.CompletableFuture.timedGet(CompletableFuture.java:1960)
	at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2095)

✖ io.quarkus.it.resteasy.reactive.kotlin.FlowResourceTest.testSeeJson line 36 - More details - Source on GitHub

java.util.concurrent.TimeoutException
	at java.base/java.util.concurrent.CompletableFuture.timedGet(CompletableFuture.java:1960)
	at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2095)

@stuartwdouglas stuartwdouglas force-pushed the delegate-back-to-io-thread branch from 649d2c7 to e54abc3 Compare September 29, 2021 06:59
stuartwdouglas
stuartwdouglas commented Sep 29, 2021
View reviewed changes
.../src/main/kotlin/org/jboss/resteasy/reactive/server/runtime/kotlin/FlowToPublisherHandler.kt
requestContext.resume()
//run in a direct invocation executor to run the rest of the invocation in the co-route scope
//feels a bit fragile, but let's see how it goes
requestContext.resume(Executor { it.run() })
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geoand I needed to do this to make the kotlin tests work. Not sure if I like it much but I think it is probably ok for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can live with it :)

@geoand geoand merged commit 35cd9a6 into quarkusio:main Sep 29, 2021
@quarkus-bot quarkus-bot bot added this to the 2.4 - main milestone Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoand geoand geoand approved these changes

Assignees
No one assigned
Labels
area/rest
Projects
None yet
Milestone
2.4.0.CR1
Development

Successfully merging this pull request may close these issues.
2 participants
@stuartwdouglas @geoand